-
Notifications
You must be signed in to change notification settings - Fork 47.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consolidate events between EventConstants and BrowserEventEmitter #9512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just a couple inline notes. Happy to merge after you fix them.
docs/js/react-dom.js
Outdated
@@ -350,7 +350,7 @@ function extractCompositionEvent(topLevelType, targetInst, nativeEvent, nativeEv | |||
} | |||
|
|||
/** | |||
* @param {string} topLevelType Record from `EventConstants`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revert this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
||
var getVendorPrefixedEventName = require('getVendorPrefixedEventName'); | ||
|
||
export type PropagationPhases = 'bubbled' | 'captured'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rollover from EventConstants. Happy to remove though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, it gets referenced here: https://github.com/facebook/react/pull/9512/files#diff-1ca53ffa4da75a64f7c3b2f131f62da7R21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just move it to that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in caccf43
eef542a
to
1c00c12
Compare
I think everything should be good to go. Struggling with CI however. Any ideas? |
Does moving the file to src/renderers/dom/shared/event/BrowserEventConstants.js fix the issue? That is where it should be anyway. |
This is a follow up to facebook#9333. This commit removes some duplication of event names and renames the EventConstants module to BrowserEventConstants.
Fixes an issue during build.
b8c7a63
to
31e8988
Compare
@spicyj Hey, that did it! I just upstreamed from master, but looks like that was it! |
Thanks! |
This is a follow up to the request made by @spicyj in #9333. This commit removes some duplication of event names by consolidating them into the EventConstants module. It then renames that module to BrowserEventConstants.